Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(bpdm-gate): Adding a currentness attribute to the business partner input re… #986

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

kunyao-cofinity-x
Copy link

@kunyao-cofinity-x kunyao-cofinity-x commented Jul 3, 2024

…quest

Description

The BPDM gate requires functionality to handle data updates from the customer side based on timestamps. This feature ensures that the data is only updated if the new data from the customer has a more recent timestamp than the existing data.

eclipse-tractusx/sig-release#726

  • Customer sends data to the gate system with a timestamp attribute (currentness).
  • The gate system receives the data and invokes the GR process.
  • The GR process compares the received timestamp with the stored timestamp.
  • If the received timestamp is newer, the data is updated, and a success message is logged and sent to the customer.
  • If the received timestamp is not newer, the update is rejected, and an informative message is logged and sent to the customer.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for a committer review:

  • Self-testing on local environment
  • Test cases creation

@kunyao-cofinity-x kunyao-cofinity-x self-assigned this Jul 3, 2024
@kunyao-cofinity-x kunyao-cofinity-x changed the title feat: Adding a currentness attribute to the business partner input re… feat(bpdm-gate): Adding a currentness attribute to the business partner input re… Jul 3, 2024
Copy link
Contributor

@SujitMBRDI SujitMBRDI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, Instant type can be used instead of Timestamp for better performance and modern API advantages, we can add DbTimestamp wrapper class to ensure truncation to microseconds and Also, refactor mapping and service function for improved readability and efficiency to ensure proper exception handling and null safety throughout the code

@@ -104,6 +105,9 @@ class BusinessPartnerDb(
@JoinColumn(name = "address_confidence_id", unique = true)
var addressConfidence: ConfidenceCriteriaDb?,

@Column(name = "currentness")
var currentness: Timestamp? = null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion from my side, since java.time.Instant is a more modern and efficient API for dealing with timestamps, it is advisable to switch to Instant.
This also avoids potential issues with different time zone handling in java.sql.Timestamp.

Also, here DbTimestamp wrapper for truncation can be created as we already did for orchestrator entities. This will ensure that timestamps are always truncated to microseconds, which can be useful if you want to avoid precision issues between application and the database.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Sujit, thanks for your suggestion. I have a question regarding using Instant. The value submitted by the customer will either be null or a String value like "2024-07-18 16:45:05", is it still suitable to use Instant in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recommendation is switch to Instant as is the best choice due to its modern design, better time zone handling, and higher precision. Handling null and string values can be easily managed with appropriate parsing and conversion logic.

@@ -124,12 +124,21 @@ class BusinessPartnerService(
val partnerToUpsert = existingPartner ?: BusinessPartnerDb.createEmpty(upsertData.sharingState, upsertData.stage)

val hasChanges = compareUtil.hasChanges(upsertData, partnerToUpsert)
val shouldUpdate = if(upsertData.currentness == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the upsertFromEntity function: Improve readability and efficiency by avoiding redundant null checks and using more idiomatic Kotlin. We can have something like below,

val hasChanges = compareUtil.hasChanges(upsertData, partnerToUpsert)
val shouldUpdate = when {
       upsertData.currentness == null -> true
       existingPartner?.currentness == null -> true
       else -> upsertData.currentness.isAfter(existingPartner.currentness)
 }

 if (hasChanges && shouldUpdate) {
       changelogRepository.save(ChangelogEntryDb(sharingState.externalId, sharingState.tenantBpnl, changeType, stage))
       copyUtil.copyValues(upsertData, partnerToUpsert)
       businessPartnerRepository.save(partnerToUpsert)
}

@nicoprow nicoprow added this to the BPDM v6.2.0 milestone Jul 24, 2024
@@ -127,12 +127,17 @@ class BusinessPartnerService(
val partnerToUpsert = existingPartner ?: BusinessPartnerDb.createEmpty(upsertData.sharingState, upsertData.stage)

val hasChanges = compareUtil.hasChanges(upsertData, partnerToUpsert)
val shouldUpdate = when {
Copy link

@Sebastian-Wurm Sebastian-Wurm Jul 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not mix the technical problem of messages / versions of the same business partner arriving in a wrong order with the currentness of the business partner. Currentness information has business value to it and must be added anyway sooner or later to the Gate as it is already available in the Pool. It may well be, that the currentness of a business partner needs to be changed, because currentness information was wrong. If the corrected currentness lies before the wrong currentness, this change would not be possible with this code. Let's remove this code here and rather introduce an integer version number at a business partner or a sequence number at the message level in another pull request. This version / sequence number could be optional for those source systems, that handle sequence problems on their own. If it is set, it must be incremented on each change of the business partner / sending of a message. Source systems not having an integer version / sequence number could rely on converting the timestamp to a unix time. This is best practice in other MDM system for decades.

Copy link

@Sebastian-Wurm Sebastian-Wurm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment regarding currentness being a business attribute which should not be abused to solve technical problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants